Skip to content

Conversation

@tzolov
Copy link
Contributor

@tzolov tzolov commented May 1, 2025

This ensures that validation errors are caught early during object construction rather than later when methods are called, providing better error feedback.

  • Add validation for tool-annotated methods during construction
  • Validate duplicate tool names in constructor instead of only at getToolCallbacks() time
  • Add comprehensive test suite for MethodToolCallbackProvider

This ensures that validation errors are caught early during object construction
rather than later when methods are called, providing better error feedback.

- Add validation for tool-annotated methods during construction
- Validate duplicate tool names in constructor instead of only at getToolCallbacks() time
- Add comprehensive test suite for MethodToolCallbackProvider

Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
@tzolov tzolov added this to the 1.0.0-RC1 milestone May 1, 2025
.toList();

if (toolMethods.isEmpty()) {
throw new IllegalStateException("No @Tool annotated methods found in " + toolObject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider extending this message to include "Did you mean to pass a ToolCallback or ToolCallbackProvider? If so, you have to use .toolCallbacks() instead of .tool()".

It could also be a warn log message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are seeing that this is a pretty critical fix, the app won't work as expected so I think an exception rather than a warn message is appropriate. agree wrt to the log message.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Also they could mean to call defaultToolCallbacks or defaultTool.

validateToolCallbacks(getToolCallbacks());
}

private void assertToolAnnotatedMethodsPresent(List<Object> toolObjects) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice!

@markpollack
Copy link
Member

merged in 6c52c99

kept the exception, can change if needed. let's see how this plays.

@markpollack markpollack closed this May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants